Fix memory leaks in addCapability and addConditionalCapability#3403
Fix memory leaks in addCapability and addConditionalCapability#3403MrSidims merged 2 commits intoKhronosGroup:mainfrom
Conversation
MrSidims
left a comment
MrSidims
left a comment
There was a problem hiding this comment.
Overall LGTM, thank you!
Small typo in commit msg: fsantize -> fsanitize
Compiling with -fsanitize=address uncovered 2 memory leaks. Instead of handling memory manually, we could use std::unique_ptr to free the memory for us.
maarquitos14
left a comment
maarquitos14
left a comment
There was a problem hiding this comment.
LGTM, just a slightly related offtopic comment.
| class SPIRVModule { | ||
| public: | ||
| typedef std::map<SPIRVCapabilityKind, SPIRVCapability *> SPIRVCapMap; | ||
| typedef std::map<SPIRVCapabilityKind, std::unique_ptr<SPIRVCapability>> |
There was a problem hiding this comment.
While we're here, do we really need to keep the order here? Could this be made std::unordered_map? That would probably give us better performance. Anyway, I know this is offtopic, so feel free to ignore or address in a different patch.
There was a problem hiding this comment.
I'll add it to my TODO list and do it in a separate PR. It's a low hanging fruit so it's worth checking.
I'll also see if I can remove the rest of the manual memory management from the class, such that the destructor becomes trivial.
There was a problem hiding this comment.
I've gave it a try and I know now why std::map is used: If we want to use the unordered containers on these two maps we have to define std::hash(const SPIRVCapabilityKind&).
5 participants
Compiling with
-fsanitize=addressuncovered 2 memory leaks related to the use of raw pointers inaddCapabilityandaddConditionalCapability.Instead of manually handling memory, use
std::unique_ptrto track the ownership of the pointed object and free the memory for us.